fix(backend): Fix JWT array audience validation#8470
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 5c2ce3b The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe pull request changes verifyJwt to call assertAudienceClaim(aud, audience) without wrapping values in arrays, and adds tests: one unit test for assertAudienceClaim that expects an error when audience arrays do not intersect, plus two verifyJwt tests that sign tokens with an array aud claim and check acceptance when the configured audience is present and rejection when it is absent. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
This could be for a follow-up but I think we could change the types in
from unknown tostring | string[]
Also I understand skipping verification if we have no expected audience, but it seems odd to skip it if we do expect something and the token doesn't provide any.
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
wobsoriano
left a comment
There was a problem hiding this comment.
@dominic-clerk approving as this fixes array aud handling and adds targeted coverage.
The type cleanup and missing aud behavior question can be a follow-up, right?
Summary
This fixes JWT audience validation for tokens whose
audclaim is an array.verifyJwtnow passes the rawaudclaim and configuredaudienceoption intoassertAudienceClaim. The assertion helper already normalizes string and string-array inputs, so this lets it evaluate array audiences correctly.Scope
This does not mean default Clerk session tokens have an
audclaim. They generally do not, and this PR does not change the existing behavior for tokens with noaud: if the token has no audience claim,verifyJwtstill skips audience validation.The bug only applies when both of these are true:
audienceaudas a string arrayThat shape is valid JWT syntax and is already represented in this package's M2M token parsing fallback (
aud?: string[]). Clerk-issued M2M tokens also allow custom claims, andaudis not reserved there. So the fix is scoped to correctly enforcing the existingaudienceoption when an arrayaudis present, not to introducing audience binding for all Clerk tokens.Root Cause
verifyJwtwrapped both values before calling the assertion:When the token already had
audas a string array, the assertion received a nested array. That shape skipped both validation branches: it was not a string, and it was not an array of strings. The function returned without enforcing the configured audience.Tests
audwhen it includes the configured audienceaudwhen it does not include the configured audienceValidation
NODE_OPTIONS=--no-experimental-webstorage pnpm --filter @clerk/backend buildpnpm --filter @clerk/backend build:runtimeNODE_OPTIONS=--no-experimental-webstorage pnpm exec vitest run src/jwt/__tests__/assertions.test.ts src/jwt/__tests__/verifyJwt.test.ts --environment node --typecheck.enabled=falsepnpm --filter @clerk/backend format:checkgit diff --check